Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow inline summary cutoff #2581

Open
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

clarfonthey
Copy link
Contributor

@clarfonthey clarfonthey commented Jul 21, 2024

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Are you doing the PR on the next branch?
  • Have you created/updated the relevant documentation page(s)?

This worked previously, but was broken by a recent change. Essentially, because the InlineHtml event is not considered for the <!-- more --> marker, it has no meaning at the moment when not placed on its own line.

This alters the code to allow this again, but does so in a more reasonable way:

  1. It properly closes tags that are left open in the summary. (Inline HTML is not considered; you're on your own if you do that.)
  2. It runs a summary-cutoff.html template at the end of the summary if this happens, which acts depending on the summary before the cutoff and the current language. It defaults to showing an ellipsis if the summary doesn't end in punctuation.

I've also added tests to verify this behaviour works.

Fixes #2562.

@Keats
Copy link
Collaborator

Keats commented Jul 21, 2024

Honestly I'm not too sure about that. The original intent when it was added was to put it on an empty line and putting it in the middle of some HTML - I don't know...

@clarfonthey
Copy link
Contributor Author

clarfonthey commented Jul 21, 2024

That's extremely fair. In my case, I don't use it very often, but the current case where I do use it is for cutting sentences off as a kind of cliffhanger. Things like "So, I took a look at X, and <cutoff>". I just made a complete version that accounts for all kinds of markdown syntax because if I was going to do anything, I might as well do it right instead of just adding a </p> sometimes. (Which is what my code did before this was properly supported.)

And like, I guess that there might be cases where you might want to cut off in the middle of a blockquote or something, although those are probably much rarer.

If you do want it to remain on its own line, I can remove the summary-specific stuff and just keep it to fixing shortcodes for now. I do think that <span>{{ code() }}</span> not working and {{ code() }} working by itself is a little weird.

@nyanpasu64
Copy link
Contributor

Not sure about the technical details of this PR (I haven't looked into the code), but I found that even <!-- more --> in the middle or end of a regular Markdown paragraph (without HTML elements) is no longer detected as a marker. I use this heavily in my blog (https://nyanpasu64.gitlab.io/), including breaking summaries in the middle of a paragraph, and cannot update to 0.19.x until this is fixed. Is this related to this PR?

@clarfonthey
Copy link
Contributor Author

Short answer, yes,

Long answer, yes but it will change the code so that it actually emits the end of the paragraph and leave a cutoff marker in its place, so, you will need to modify your templates if they were manually adding the paragraph end marker back (alongside an ellipsis) in some capacity.

@LunarEclipse363
Copy link
Contributor

LunarEclipse363 commented Aug 7, 2024

  1. It leaves a <span class="summary-cutoff"></span> element before those tags are closed so it can be properly styled. This is slightly more portable than just using an ellipsis, since you can choose what character to use via CSS instead.

This could potentially be a built-in template the user can override like anchor-link.html.

The issue with putting the ellipsis in via CSS is that some user agents (Atom/RSS feed readers for example) might not load the stylesheet at all and then the ellipsis is gone entirely. I'm also not sure what implications would it have for accessibility.

@clarfonthey
Copy link
Contributor Author

This could potentially be a built-in template the user can override like anchor-link.html.

Actually, yeah, that seems the most reasonable. Could also provide the template with the contents of the summary so you could, for example, only emit an ellipsis if there isn't punctuation at the end of the line. I'll make that the default.

@clarfonthey
Copy link
Contributor Author

Marking this as draft since I'm moving the shortcode fixes to a separate PR. Will rebase on top of that once it merges.

@clarfonthey clarfonthey marked this pull request as draft August 13, 2024 04:32
@clarfonthey clarfonthey changed the title Fix shortcode/continue-reading parsing with inline HTML Allow inline summary cutoff Aug 15, 2024
@clarfonthey clarfonthey force-pushed the continue-reading-inline branch 3 times, most recently from 4db3353 to ef3ced0 Compare August 15, 2024 18:50
@clarfonthey clarfonthey marked this pull request as ready for review August 15, 2024 18:50
@clarfonthey
Copy link
Contributor Author

Rebased this on the other change and added a template version of the summary cutoff. By default, it ends the summary in an ellipsis if it was cut off inline.

@@ -793,15 +796,50 @@ pub fn markdown_to_html(
.position(|e| matches!(e, Event::Html(CowStr::Borrowed(CONTINUE_READING))))
.unwrap_or(events.len());

// determine closing tags missing from summary
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we error if the summary marker is inside a tag? Eg <i <-- more --> class='' />

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since that's just straight-up invalid HTML, I'd be more inclined to just make the <!-- more --> regexp only match if it's by itself (i.e. not nested inside something like that). Will add a test for it.

Personally, I think that an error would be weird; we don't emit errors for invalid HTML otherwise, so, it's better to just ignore it and treat it like otherwise malformed HTML.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I tried adding a test for this, but the markdown parser just isn't happy with it. If it's valid HTML, it parses, and if it's not, it just escapes it. So, in your example, it shows up as &lt;i at the end of the summary.

I'm inclined to just say that's okay, honestly. We could detect if it's inside any custom HTML tags, like <i><!-- MORE --></i>, but that goes against the principle that we aren't accounting for custom HTML tags at all and those just remain opened.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good

&None,
)
.context("Failed to render summary cutoff template")?;
summary_html.push_str(&summary_cutoff);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want a template or a string in the config? Template is more powerful but might be overkill?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like the template is ideal, although it's unlikely many people will change the template. The original idea was proposed as an easy option since even anchor-link.html has a simple template associated with it.

Without a template, you can't choose whether to emit an ellipsis based upon the closing punctuation, so, I'm inclined to say that the template is strictly better.

Copy link
Collaborator

@Keats Keats left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit but good otherwise

@@ -0,0 +1 @@
{% if summary is matching("\PP$") %}&hellip;{% endif %}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we default to an ellipsis without the test and mention that in the docs as an example?
matching will currently create a new regex everytime this template is called so it's not ideal perf wise as a default

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I had no idea that was the case; I'm fine with that, then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, no idea where Tera 2 is currently, but I'd be willing to help figure out how to solve this problem there. Feels like it should be doable to let the functions know about the constant arguments and optimize themselves accordingly.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible to fix in tera 1 I think yes, just have a cache for the compiled regexes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants